[FAL-2076] use high priority queue for celery heartbeat check by pomegranited · Pull Request #28034 · openedx/openedx-platform

pomegranited · 2021-06-25T04:51:08Z

Description

This change fixes the default celery queue used for heartbeat checks in production, setting the default to the high-priority queue for LMS/Studio, respectively.

Currently, the HEARTBEAT_CELERY_ROUTING_KEY defaults to the HIGH_PRIORITY_QUEUE set in lms.env.common, which is a generic 'edx.core.high' queue name.

Unfortunately, since both the LMS and Studio env.production settings files override HIGH_PRIORITY_QUEUE to be 'edx.lms.core.high' (and edx.cms.core.high respectively) before defining the list of CELERY_QUEUES, the 'edx.core.high' queue never gets created, and so the celery heartbeat check fails.

This change marks HEARTBEAT_CELERY_ROUTING_KEY as a derived setting, so that it can be lazily defaulted to the service variant's HIGH_PRIORITY_QUEUE.

Native default master settings (without this change)

# LMS
$ /edx/bin/edxapp-shell-lms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.lms.core.high'
>>> settings.CELERY_QUEUES
{'edx.lms.core.default': {}, 'edx.lms.core.high': {}, 'edx.lms.core.high_mem': {}, 'edx.cms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AssertionError

# Studio
$ /edx/bin/edxapp-shell-cms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.cms.core.high'
>>> settings.CELERY_QUEUES
{'edx.cms.core.default': {}, 'edx.cms.core.high': {}, 'edx.lms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AssertionError

Native default master settings (with this change)

# LMS
$ /edx/bin/edxapp-shell-lms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.lms.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.lms.core.high'
>>> settings.CELERY_QUEUES
{'edx.lms.core.default': {}, 'edx.lms.core.high': {}, 'edx.lms.core.high_mem': {}, 'edx.cms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
>>>

# Studio
$ /edx/bin/edxapp-shell-cms 
>>> from django.conf import settings
>>> settings.HEARTBEAT_CELERY_ROUTING_KEY
'edx.cms.core.high'
>>> settings.HIGH_PRIORITY_QUEUE
'edx.cms.core.high'
>>> settings.CELERY_QUEUES
{'edx.cms.core.default': {}, 'edx.cms.core.high': {}, 'edx.lms.core.default': {}}
>>> assert settings.HEARTBEAT_CELERY_ROUTING_KEY in settings.CELERY_QUEUES
>>>

Supporting information

This issue was introduced by https://github.com/edx/edx-platform/pull/23731 (edx@73fd2ed) and is present in koa.master, lilac.master, and master.

Testing instructions

Sandbox URL

Extra settings:

HEARTBEAT_EXTENDED_CHECKS:
  - openedx.core.djangoapps.heartbeat.default_checks.check_celery
  - openedx.core.djangoapps.django_comment_common.comment_client.utils.check_forum_heartbeat

Ensure that the celery extended heartbeat check is passing for the sandbox.

Deadline

No deadline on our side, but it could be affecting many people, so we'd like to get it merged ASAP.

Other information

We would expect edx.org's worker servers to exhibit this issue too. Are edx.org's workers not using the extended heartbeat check? Or perhaps edx.org overrides HEARTBEAT_CELERY_ROUTING_KEY? courses.edx.org fails the celery extended heartbeat check.

openedx-webhooks · 2021-06-25T04:51:13Z

Thanks for the pull request, @pomegranited! I've created OSPR-5883 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

gabor-boros

👍 🎉

I tested what's in the test instructions
I read through the code
~~I checked for accessibility issues~~
~~Includes documentation~~

natabene · 2021-06-30T20:32:07Z

@pomegranited Thank you for your contribution. Is this good for our review?

pomegranited · 2021-07-01T02:42:30Z

@natabene Yep, ready for edx/community review. Thanks @gabor-boros !

Pinging @felipemontoya @ziafazal @bradenmacdonald for a core committer review, and @arbrandes FYI since we'll need this in lilac too.

felipemontoya · 2021-07-01T03:02:56Z

Hey @pomegranited this is great. I did not know about the ?extended heartbeat.

In this case would it help to use derived to make the setting defined in common be up to date with the overrides given in production?

Reverts previous fix. Makes HIGH_PRIORITY_QUEUE a derived setting, which allows HEARTBEAT_CELERY_ROUTING_KEY to use the correct config variant default. Adds test.

pomegranited · 2021-07-01T04:03:43Z

@felipemontoya Oo I don't know about derived settings either!

I've done that with 0940b02, but since it adds a function, also had to add a test (and update the test settings). Will see whether that affects anything else?

FYI, we generally override HEARTBEAT_EXTENDED_CHECKS to also run a forum heartbeat check, to make sure the discussion forum is working as expected.

HEARTBEAT_EXTENDED_CHECKS:
  - openedx.core.djangoapps.heartbeat.default_checks.check_celery
  - openedx.core.djangoapps.django_comment_common.comment_client.utils.check_forum_heartbeat

edx-status-bot · 2021-07-01T07:50:10Z

Your PR has finished running tests. There were no failures.

arbrandes · 2021-07-01T12:19:33Z

@pomegranited, ACK! Scheduling a task to look into it once its merged.

pomegranited · 2021-07-06T06:31:23Z

@felipemontoya I've applied your suggestions and added the necessary tests, and my sandbox is running with the latest code and working as expected. Are you able to review and merge this fix as core committer?

felipemontoya · 2021-07-06T17:55:31Z

Hey @pomegranited, I was queuing this until I could load it in a environment running master. However I have not had the time to do it.

In terms of the approach and a static code analysis it looks correct to me and as a Core Commiter I would approve it.

pomegranited · 2021-07-06T22:40:24Z

Thanks @felipemontoya ! For our scheduling, do you have an idea of when you'll be able to test this?

felipemontoya · 2021-07-08T20:21:34Z

I'm very sorry @pomegranited, I don't know when I will be able to review. As I said, from a static read I am good with the approach.

Perhaps a different CC can take this PR from here.

pomegranited · 2021-07-09T01:25:30Z

No worries @felipemontoya , thanks for letting me know!
Pinging the other CCs on edx-platform to see if any of you have time to review this? @ziafazal @bradenmacdonald @pdpinch

bradenmacdonald · 2021-07-16T15:53:24Z

@pomegranited I think this may already be fixed by @feanil in #28066 - can you please check? If not, I can take this review.

pomegranited · 2021-07-19T02:57:24Z

@bradenmacdonald Yep, the master periodic build heartbeat is working fine now, so https://github.com/edx/edx-platform/pull/28066 addressed this. I should have checked for open PRs before doing the rework here, and saved some duplicated effort!

openedx-webhooks · 2021-07-19T02:57:35Z

@pomegranited Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

fix: use high priority queue for celery heartbeat check

f483351

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jun 25, 2021

gabor-boros approved these changes Jun 30, 2021

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Jul 1, 2021

refactor: use derived settings instead

0940b02

Reverts previous fix. Makes HIGH_PRIORITY_QUEUE a derived setting, which allows HEARTBEAT_CELERY_ROUTING_KEY to use the correct config variant default. Adds test.

pomegranited added 3 commits July 1, 2021 14:51

chore: fix quality and cms-unit test

77af33d

Merge branch 'master' into jill/fix-heartbeat-celery-routing-key

32f8467

chore: fix quality and tests

5cfece2

openedx-webhooks added the awaiting prioritization label Jul 3, 2021

pomegranited mentioned this pull request Jul 6, 2021

[FAL-2076] Cherry-pick celery fixes on Koa.3 open-craft/openedx-platform#367

Merged

1 task

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 8, 2021

pomegranited closed this Jul 19, 2021

openedx-webhooks added rejected and removed awaiting prioritization labels Jul 19, 2021

pomegranited mentioned this pull request Sep 29, 2021

Cherry-picks #28066 into koa.master #28869

Merged

2 tasks

pomegranited deleted the jill/fix-heartbeat-celery-routing-key branch November 1, 2024 02:14

Uh oh!

Conversation

pomegranited commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Other information

Uh oh!

openedx-webhooks commented Jun 25, 2021

Uh oh!

gabor-boros left a comment

Choose a reason for hiding this comment

Uh oh!

natabene commented Jun 30, 2021

Uh oh!

pomegranited commented Jul 1, 2021

Uh oh!

felipemontoya commented Jul 1, 2021

Uh oh!

pomegranited commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edx-status-bot commented Jul 1, 2021

Uh oh!

arbrandes commented Jul 1, 2021

Uh oh!

pomegranited commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felipemontoya commented Jul 6, 2021

Uh oh!

pomegranited commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felipemontoya commented Jul 8, 2021

Uh oh!

pomegranited commented Jul 9, 2021

Uh oh!

bradenmacdonald commented Jul 16, 2021

Uh oh!

pomegranited commented Jul 19, 2021

Uh oh!

openedx-webhooks commented Jul 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pomegranited commented Jun 25, 2021 •

edited

Loading

pomegranited commented Jul 1, 2021 •

edited

Loading

pomegranited commented Jul 6, 2021 •

edited

Loading

pomegranited commented Jul 6, 2021 •

edited

Loading